Handle closures correctly in MIR inlining #45913

sinkuu · 2017-11-10T14:26:40Z

Fixes #45894.

shepmaster · 2017-11-10T14:52:53Z

Auto-assigning to...

r? @nikomatsakis

nikomatsakis

Looks great! I left a few style nits.

nikomatsakis · 2017-11-10T15:55:24Z

src/librustc_mir/transform/inline.rs

-        args.into_iter().map(|a| {
-            if let Operand::Consume(Lvalue::Local(local)) = a {
+
+        fn create_temp_if_necessary<'a, 'tcx: 'a>(


Nit: Could we move this to a method (and then get rid of the tcx argument)? (I just find it harder to read things "nested" funtions, that's all.)

Also, good opportunity to add a comment (the surrounding code is a bit...comment-poor). Perhaps something like:

/// If `arg` is already a temporary, returns it. Otherwise, introduces a fresh /// temporary `T` and an instruction `T = arg`, and returns `T`.

(Does that sound accurate?)

Finally, can we change the return value to Lvalue, to reflect the fact that this always returns an Operand::Consume?

nikomatsakis · 2017-11-10T15:55:52Z

src/librustc_mir/transform/inline.rs

+
+        // A closure is passed its self-type and a tuple like `(arg1, arg2, ...)`,
+        // hence mappings to tuple fields are needed.
+        if tcx.def_key(callsite.callee).disambiguated_data.data == DefPathData::ClosureExpr {


Nit: let's do if let DefPathData::ClosureExpr = tcx.def_key(callsite.callee).disambiguated_data.data

better yet, let's add a is_closure(def_id: DefId) helper to the tcx (and call it from closure_base_def_id)

nikomatsakis · 2017-11-10T15:59:21Z

src/librustc_mir/transform/inline.rs

+
+            let self_ = create_temp_if_necessary(args.next().unwrap(), tcx, callsite, caller_mir);
+
+            let tuple = if let Operand::Consume(lvalue) =


(if we changed return type, this if let would not be necessary)

nikomatsakis · 2017-11-10T18:35:25Z

src/librustc_mir/transform/check_unsafety.rs

+    // closures are handled by their parent fn.
+    if tcx.is_closure(def_id) {
+        return;
+    }


nice drive-by cleanup =)

nikomatsakis · 2017-11-10T18:35:52Z

@bors r+

bors · 2017-11-10T18:35:52Z

📌 Commit 17cbd9f has been approved by nikomatsakis

sinkuu · 2017-11-14T08:44:28Z

Rebased.

arielb1 · 2017-11-14T10:23:28Z

@bors r+

bors · 2017-11-14T10:23:29Z

📌 Commit ec2ff8f has been approved by arielb1

bors · 2017-11-14T11:28:20Z

⌛ Testing commit ec2ff8f with merge 1cfe6599667b4f0c978e0d911988a39e76340f58...

bors · 2017-11-14T12:38:09Z

💔 Test failed - status-travis

kennytm · 2017-11-14T12:48:13Z

src/test/mir-opt/inline-closure.rs

+//     StorageLive(_6);
+//     StorageLive(_7);
+//     _7 = _2;
+//     _6 = Mul(_7, const 2i32);


Checked arithmetic strikes again! These are CheckedMul in debug build. Please change to a non-checked operator like q & 3 or maybe just write x(q, q).

[01:06:23] ---- [mir-opt] mir-opt/inline-closure.rs stdout ---- [01:06:23] thread '[mir-opt] mir-opt/inline-closure.rs' panicked at 'Did not find expected line, error: Mismatch in lines [01:06:23] Currnt block: None [01:06:23] Expected Line: " _8 = CheckedMul(_7, const 2i32); // scope 1 at /checkout/src/test/mir-opt/inline-closure.rs:21:7: 21:10" [01:06:23] Actual Line: " _6 = Mul(_7, const 2i32);" [01:06:23] Expected: [01:06:23] ... (elided) [01:06:23] ... (elided) [01:06:23] bb0: { [01:06:23] StorageLive(_3); [01:06:23] _3 = [closure@NodeId(28)]; [01:06:23] StorageLive(_4); [01:06:23] _4 = &_3; [01:06:23] StorageLive(_5); [01:06:23] StorageLive(_6); [01:06:23] StorageLive(_7); [01:06:23] _7 = _2; [01:06:23] _6 = Mul(_7, const 2i32); [01:06:23] StorageDead(_7); [01:06:23] StorageLive(_8); [01:06:23] StorageLive(_9); [01:06:23] _9 = _2; [01:06:23] _8 = Mul(_9, const 3i32); [01:06:23] StorageDead(_9); [01:06:23] _5 = (_6, _8); [01:06:23] _0 = (_5.0: i32); [01:06:23] StorageDead(_5); [01:06:23] StorageDead(_8); [01:06:23] StorageDead(_6); [01:06:23] StorageDead(_4); [01:06:23] StorageDead(_3); [01:06:23] return; [01:06:23] } [01:06:23] ... (elided) [01:06:23] Actual: [01:06:23] fn foo(_1: T, _2: i32) -> i32 { [01:06:23] let mut _0: i32; [01:06:23] scope 1 { [01:06:23] let _3: [closure@NodeId(28)]; [01:06:23] scope 3 { [01:06:23] } [01:06:23] } [01:06:23] scope 2 { [01:06:23] } [01:06:23] let mut _4: &[closure@NodeId(28)]; [01:06:23] let mut _5: (i32, i32); [01:06:23] let mut _6: i32; [01:06:23] let mut _7: i32; [01:06:23] let mut _8: (i32, bool); [01:06:23] let mut _9: i32; [01:06:23] let mut _10: i32; [01:06:23] let mut _11: (i32, bool); [01:06:23] bb0: { [01:06:23] StorageLive(_3); [01:06:23] _3 = [closure@NodeId(28)]; [01:06:23] StorageLive(_4); [01:06:23] _4 = &_3; [01:06:23] StorageLive(_5); [01:06:23] StorageLive(_6); [01:06:23] StorageLive(_7); [01:06:23] _7 = _2; [01:06:23] _8 = CheckedMul(_7, const 2i32); [01:06:23] assert(!(_8.1: bool), "attempt to multiply with overflow") -> bb1; [01:06:23] } [01:06:23] bb1: { [01:06:23] _6 = (_8.0: i32); [01:06:23] StorageDead(_7); [01:06:23] StorageLive(_9); [01:06:23] StorageLive(_10); [01:06:23] _10 = _2; [01:06:23] _11 = CheckedMul(_10, const 3i32); [01:06:23] assert(!(_11.1: bool), "attempt to multiply with overflow") -> bb2; [01:06:23] } [01:06:23] bb2: { [01:06:23] _9 = (_11.0: i32); [01:06:23] StorageDead(_10); [01:06:23] _5 = (_6, _9); [01:06:23] _0 = (_5.0: i32); [01:06:23] StorageDead(_5); [01:06:23] StorageDead(_9); [01:06:23] StorageDead(_6); [01:06:23] StorageDead(_4); [01:06:23] StorageDead(_3); [01:06:23] return; [01:06:23] } [01:06:23] }', /checkout/src/tools/compiletest/src/runtest.rs:2337:12 [01:06:23] note: Run with `RUST_BACKTRACE=1` for a backtrace. [01:06:23] [01:06:23] [01:06:23] failures: [01:06:23] [mir-opt] mir-opt/inline-closure.rs [01:06:23] [01:06:23] test result: �[31mFAILED�(B�[m. 44 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out

arielb1 · 2017-11-14T14:04:20Z

@bors r+

bors · 2017-11-14T14:04:21Z

📌 Commit a1f7bad has been approved by arielb1

bors · 2017-11-15T01:32:37Z

⌛ Testing commit a1f7bad with merge 8a98531...

Handle closures correctly in MIR inlining Fixes #45894.

bors · 2017-11-15T03:57:13Z

☀️ Test successful - status-appveyor, status-travis
Approved by: arielb1
Pushing 8a98531 to master...

shepmaster added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Nov 10, 2017

rust-highfive assigned nikomatsakis Nov 10, 2017

nikomatsakis requested changes Nov 10, 2017

View reviewed changes

nikomatsakis reviewed Nov 10, 2017

View reviewed changes

nikomatsakis approved these changes Nov 10, 2017

View reviewed changes

kennytm added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 10, 2017

sinkuu added 3 commits November 14, 2017 17:12

Handle closures correctly in MIR inlining

cc36f88

Make create_temp_necessary a method

c3ec175

Add TyCtxt::is_closure

ec2ff8f

sinkuu force-pushed the mir-inlining-closure branch from 17cbd9f to ec2ff8f Compare November 14, 2017 08:44

kennytm reviewed Nov 14, 2017

View reviewed changes

kennytm added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 14, 2017

Fix test

a1f7bad

kennytm added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 14, 2017

bors added a commit that referenced this pull request Nov 15, 2017

Auto merge of #45913 - sinkuu:mir-inlining-closure, r=arielb1

8a98531

Handle closures correctly in MIR inlining Fixes #45894.

bors merged commit a1f7bad into rust-lang:master Nov 15, 2017


		let self_ = create_temp_if_necessary(args.next().unwrap(), tcx, callsite, caller_mir);

		let tuple = if let Operand::Consume(lvalue) =

Handle closures correctly in MIR inlining #45913

Handle closures correctly in MIR inlining #45913

Uh oh!

Conversation

sinkuu commented Nov 10, 2017

Uh oh!

shepmaster commented Nov 10, 2017

Uh oh!

nikomatsakis left a comment

Choose a reason for hiding this comment

Uh oh!

nikomatsakis Nov 10, 2017

Choose a reason for hiding this comment

Uh oh!

nikomatsakis Nov 10, 2017

Choose a reason for hiding this comment

Uh oh!

nikomatsakis Nov 10, 2017

Choose a reason for hiding this comment

Uh oh!

nikomatsakis Nov 10, 2017

Choose a reason for hiding this comment

Uh oh!

nikomatsakis Nov 10, 2017

Choose a reason for hiding this comment

Uh oh!

nikomatsakis Nov 10, 2017

Choose a reason for hiding this comment

Uh oh!

nikomatsakis Nov 10, 2017

Choose a reason for hiding this comment

Uh oh!

nikomatsakis commented Nov 10, 2017

Uh oh!

bors commented Nov 10, 2017

Uh oh!

sinkuu commented Nov 14, 2017

Uh oh!

arielb1 commented Nov 14, 2017

Uh oh!

bors commented Nov 14, 2017

Uh oh!

bors commented Nov 14, 2017

Uh oh!

bors commented Nov 14, 2017

Uh oh!

kennytm Nov 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arielb1 commented Nov 14, 2017

Uh oh!

bors commented Nov 14, 2017

Uh oh!

bors commented Nov 15, 2017

Uh oh!

bors commented Nov 15, 2017

Uh oh!

Uh oh!

kennytm Nov 14, 2017 •

edited

Loading